PR Review — Golden Image Regression Test

Critical Bug: CPUBackend.cpp include will break GoldenRenderer build

CPUBackend.cpp:5 includes "Peanut/Random.h" unconditionally, but nothing from that header is used — all RandomFloat calls are already qualified as PathTracerCore::RandomFloat (from PathTracerCore.h). The GoldenRenderer target does not add the Peanut/ include path needed to resolve Peanut/Random.h, so the CI golden-test job will fail at compile time.

Fix: Remove line 5 of CPUBackend.cpp.

Medium: GoldenRenderer never calls RecalculateView()

GoldenRenderer.cpp:112-116:

Camera camera(45.0f, 0.1f, 100.0f);
// ...
camera.OnResize(width, height);

RecalculateView() is only called inside Camera::OnUpdate() (now #ifdef GOLDEN_RENDERER-guarded out at Camera.cpp:22-99). m_InverseView stays as identity matrix (glm::mat4{1.0f}).

This happens to be accidentally correct for this scene because:

• Camera::Camera() sets m_ForwardDirection = (0,0,-1), which aligns with the default -Z view direction
• RecalculateRayDirections() transforms ray directions with w=0, so the translational component of the view matrix has no effect

But if anyone changes the camera constructor's forward direction or position, this will silently break. Fix: Add camera.RecalculateView(); after camera construction and before OnResize().

Low: emissive_scene test config misleading

test_golden.py:35-40 defines an emissive_scene entry with different resolution/frames/bounces, but GoldenRenderer has no --scene flag — both "scenes" render the same geometry. The name implies a different scene configuration. Either:

• Remove the second scene config (or rename it to e.g. default_scene_lowres)
• Or add a --scene argument to GoldenRenderer that selects between different scene setups

Low: stbi_flip_vertically_on_write modifies global state

GoldenRenderer.cpp:130 — the flip call modifies global stb state. If the binary ever writes two PNGs in one run, the second write would be double-flipped. Not an issue today (single output) but fragile.

Summary

Issues 1 and 2 should be fixed before merge. Issue 3 is cosmetic but worth addressing for clarity. Everything else looks solid — the Python script is well-structured, the CI integration is clean, and the uv-based dependency management is a good choice.

opencode session  |  github run

修复状态

Here is my review of the PR, covering both the previously flagged issues and new findings.

PR Review: Phase 1 — Golden Image Regression Test

1. Bug: Duplicate includes in CPUBackend.cpp:5-6 (Severity: Medium)

The commit bbf0102 ("fix: review feedback — dead include") replaced #include "Peanut/Random.h" with duplicates of PathTracerCore.h and Constants.h that were already present at lines 3-4:

3: #include "PathTracerCore.h"   // original
4: #include "Constants.h"         // original
5: #include "PathTracerCore.h"   // duplicate — accidentally added by "fix"
6: #include "Constants.h"         // duplicate — accidentally added by "fix"

The correct fix was to simply delete line 5 (#include "Peanut/Random.h") with zero additions. Remove lines 5-6.

2. Missing RecalculateView() in GoldenRenderer.cpp:116 (Severity: Medium)

Previously flagged and acknowledged. OnResize() calls RecalculateProjection() and RecalculateRayDirections(), but never RecalculateView(). m_InverseView stays as glm::mat4{1.0f}. This is correct by accident because:

• Camera defaults to m_ForwardDirection = (0,0,-1), which is identity rotation
• RecalculateRayDirections() transforms rays with w=0, so the translational component of m_InverseView is irrelevant

However, if anyone ever changes the camera's default forward direction or adds a constructor overload, ray directions will silently become wrong. Add camera.RecalculateView() after construction (line 112) to make the dependency explicit.

3. ISPC vs C++ fallback RNG divergence (Severity: Medium) — new finding

The golden reference images were generated locally with ISPC enabled (SSIM=1.0, MSE=0.0). But the CI golden-test job has no ISPC setup (vendor/ispc/bin/ispc.exe doesn't exist), so the build falls back to the C++ RenderCPUFallback. The RandomFloat implementations differ between the two paths:

Even though both use the same PCG_Hash algorithm, the different float conversions produce different random sequences, potentially different rendered images. With 100 accumulated frames the images will look similar (SSIM likely still ≥ 0.999), but MSE won't be exactly 0 and the test could sporadically fail in CI.

Fix options:

• Install ISPC in the CI golden-test job (add a step to download ispc.exe from GitHub releases)
• Or generate golden images with PN_ISPC disabled (pass a build flag to skip the ISPC check)

4. Dead build config for GoldenRenderer target (Severity: Low)

xmake.lua:212-213 adds add_includedirs("Peanut/Peanut/src") and add_defines("PN_PLATFORM_WINDOWS"). With GOLDEN_RENDERER defined:

• Camera.cpp gates out all Peanut includes (#ifndef GOLDEN_RENDERER)
• CPUBackend.cpp, BVH.cpp, GoldenRenderer.cpp include no Peanut headers
• PN_PLATFORM_WINDOWS is not consumed by any source file in this target

Both are dead code. Remove them.

5. SSIM includes alpha channel (Severity: Low)

test_golden.py:93-94 passes 4-channel RGBA images to skimage.metrics.structural_similarity with channel_axis=2. The alpha channel (always 255) contributes ~25% weight to the SSIM score with zero useful signal, diluting the metric's sensitivity. Consider slicing to RGB only:

ssim_val = ssim(img[:,:,:3], ref[:,:,:3], channel_axis=2, data_range=255)

6. uv.lock is platform-specific (Severity: Very Low)

The committed 1042-line uv.lock was generated on Windows. If tests are ever run on Linux/macOS, uv sync will fail or need lock regeneration. Not a current CI issue (runner is windows-2025-vs2026), but worth documenting.

Summary

Issues 1, 2, and 3 are the most important to fix before merge. Issue 3 is the highest risk — a deterministic CI regression test must have a deterministic renderer configuration matching the golden image generation.

opencode session  |  github run

第二轮审查修复 (9961632)

CI 新机制：GoldenRenderer 在同一 CI 环境内渲染两次，比对自洽性（SSIM 阈值 0.9999）。消除跨机器/MSVC 版本浮点差异问题。

Here is my review of this PR, covering previously flagged issues (verifying fixes) and new findings.

PR Review: Phase 1 — Golden Image Regression Test

Previously Flagged Issues — Status Verification

New Findings

7. Camera.h:11 declares OnUpdate() but GOLDEN_RENDERER builds have no implementation (Severity: Medium)

Camera.h:11 declares bool OnUpdate(float ts) unconditionally. But Camera.cpp:22-99 wraps the entire implementation in #ifndef GOLDEN_RENDERER. When GOLDEN_RENDERER is defined, the linker symbol is undefined. Currently nothing calls it in the GoldenRenderer, so it's invisible — but a linker error time bomb if anyone ever references it. Guard the declaration or provide a no-op stub:

// Camera.h:11 — guard the declaration
#ifdef GOLDEN_RENDERER
    bool OnUpdate(float) { return false; }
#else
    bool OnUpdate(float ts);
#endif

8. CPUBackend.cpp:279-281 — Structured binding captures unused Position and Radius (Severity: Low)

const auto& [Position, Radius, MaterialIndex] = m_ActiveScene->Spheres[ObjectIndex];

Position and Radius are never used — only MaterialIndex is consumed on line 283. This is a pre-existing issue (the hit point computation was moved into ClosestHit during BVH refactoring), but with /W4 on MSVC this may generate C4189 (unused local variable) warnings. Consider using std::ignore-style bindings or a comment-silence pattern if compiler diagnostics are a concern.

9. Camera.cpp:74 shadows Camera::kUpDirection with identical local constexpr (Severity: Very Low)

constexpr glm::vec3 kUpDir(0.0f, 1.0f, 0.0f);  // shadows Camera::kUpDirection

Both are (0,1,0), so no semantic bug, but confusing to have two identically-valued constants with different names in the same class scope. Pre-existing, not introduced by this PR.

10. CPUBackend::Render() modifies m_AccumFrameIndex then launches std::execution::par reading it (Severity: Very Low)

At CPUBackend.cpp:49, m_AccumFrameIndex = frameIndex is written before RenderCPUFallback (which reads it in parallel lambdas). On x64, aligned uint32_t reads are tear-free. This is safe in practice, but undocumented implicit contract. Fine for now.

Summary

7 — Medium    Camera.h OnUpdate() declaration without GOLDEN_RENDERER implementation
8 — Low       Unused structured binding captures Position/Radius (pre-existing)
9 — Very Low  Duplicate constexpr up-direction in Camera.cpp (pre-existing)
10 — Very Low Implicit atomic contract on m_AccumFrameIndex (edge, harmless)

The core design — CI self-consistency instead of cross-machine golden comparison — is the right call given ISPC vs C++ RNG divergence. The --ci mode (render twice, compare SSIM >= 0.9999) eliminates the determinism concern entirely. The test_golden.py script structure is clean, and the CI integration (golden-test job) is properly isolated.

Recommendation: Fix issue 7 (the header guard) before merge. Issues 8-10 are pre-existing and non-blocking.

opencode session  |  github run